Skip to content

feat: use devalue for pushState/replaceState#14129

Open
GauBen wants to merge 6 commits into
sveltejs:mainfrom
GauBen:feat/pushstate-devalue
Open

feat: use devalue for pushState/replaceState#14129
GauBen wants to merge 6 commits into
sveltejs:mainfrom
GauBen:feat/pushstate-devalue

Conversation

@GauBen

@GauBen GauBen commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Hi!

Closes #14128: pushState/replaceState now use devalue+transport to serialize custom objects

I kept it simple, not sure if the idea will be accepted


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@changeset-bot

changeset-bot Bot commented Aug 6, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5464cfe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@svelte-docs-bot

Copy link
Copy Markdown

@GauBen
GauBen force-pushed the feat/pushstate-devalue branch from e47baa8 to ac02814 Compare August 6, 2025 21:04
@Rich-Harris

Rich-Harris commented Aug 7, 2025

Copy link
Copy Markdown
Member

I don't think we can do this. Today, if you try and do something like this...

<script>
  import { page } from '$app/state';
  import { pushState } from '$app/navigation';
</script>

<section>
  <button
    onclick={() => {
      let object = $state({ count: 0 });
      pushState('/', { object });
    }}
  >
    push state
  </button>

  {#if page.state?.object}
    <p>{page.state.object.count}</p>

    <button onclick={() => (page.state.object.count += 1)}>
      increment page.state.object.count
    </button>
  {/if}
</section>

...SvelteKit will yell at you, because you can't serialize a proxy. With this PR, if you click the first button, page.state.object becomes a reactive proxy. You can increment page.state.object.count by clicking the button. But if you navigate back, and then forward, object.count will be a non-reactive deserialized object. And so on. It's basically making a promise it can't keep. I think the current approach is probably better, even if (or maybe because) it's more restrictive

@GauBen

GauBen commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

(Technically it's the browser that yells at me if I give it a proxy, devalue will happily serialize it)

Would it be acceptable to $state.snapshot the value given to pushState? For your specific example, the behavior would then be consistent before and after a history state pop (I guess)

In dev we can also add a warning that reactivity will be lost for history states if that makes the behavior less surprising

@GauBen
GauBen force-pushed the feat/pushstate-devalue branch from df5d01b to d75268c Compare August 13, 2025 12:59
Comment thread packages/kit/test/apps/basics/test/client.test.js Outdated
@GauBen

GauBen commented Aug 13, 2025

Copy link
Copy Markdown
Contributor Author

@Rich-Harris I fixed it the stupid way™, what do you think?

Well it's not that far from the fix in #14268, maybe it's not that dumb

@GauBen
GauBen force-pushed the feat/pushstate-devalue branch from 471e75e to d55c91e Compare August 17, 2025 20:24
@Rich-Harris Rich-Harris added the needs-decision Not sure if we want to do this yet, also design work needed label Aug 21, 2025
@GauBen
GauBen force-pushed the feat/pushstate-devalue branch from d55c91e to 656ff32 Compare September 30, 2025 21:34
@Rich-Harris

Copy link
Copy Markdown
Member

Quick (and long overdue — apologies) update: our latest thinking on this is that we should use IndexedDB instead of sessionStorage, since it handles these cases without needing any serialization at all (and gives us a much larger quota in the process). Groundwork in #16346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-decision Not sure if we want to do this yet, also design work needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use transport for pushState/replaceState serialization

2 participants